[WIP] src: do not enable wasm trap handler if there's not enough vmem#62132
Draft
joyeecheung wants to merge 2 commits intonodejs:mainfrom
Draft
[WIP] src: do not enable wasm trap handler if there's not enough vmem#62132joyeecheung wants to merge 2 commits intonodejs:mainfrom
joyeecheung wants to merge 2 commits intonodejs:mainfrom
Conversation
Collaborator
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62132 +/- ##
==========================================
- Coverage 89.74% 89.65% -0.10%
==========================================
Files 675 676 +1
Lines 204601 206424 +1823
Branches 39325 39537 +212
==========================================
+ Hits 183616 185065 +1449
- Misses 13273 13489 +216
- Partials 7712 7870 +158
🚀 New features to boost your workflow:
|
hubot
pushed a commit
to v8/v8
that referenced
this pull request
Mar 10, 2026
When the system does not have enough virtual memory for the wasm cage, installing the trap handler would cause any code allocating wasm memory to throw. Therefore it's useful for the embedder to know when the system doesn't have enough virtual address space to allocate enough wasm cages and in that case, skip the trap handler installation so that wasm code can at least work (even not at the maximal performance). Node.js previously has a command line option --disable-wasm-trap-handler to fully disable trap-based bound checks, this new API would allow it to adapt automatically while keeping the optimization in the happy path, since it's not always possible for end users to opt-into disabling trap-based bound checks (for example, when a VS Code Server is loaded in a remote server for debugging). Refs: nodejs/node#62132 Refs: microsoft/vscode#251777 Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233 Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Reviewed-by: Clemens Backes <clemensb@chromium.org> Cr-Commit-Position: refs/heads/main@{#105702}
Original commit message:
[api] Add V8::GetWasmMemoryReservationSizeInBytes()
When the system does not have enough virtual memory for the wasm
cage, installing the trap handler would cause any code allocating
wasm memory to throw. Therefore it's useful for the embedder to
know when the system doesn't have enough virtual address space
to allocate enough wasm cages and in that case, skip the
trap handler installation so that wasm code can at least work
(even not at the maximal performance).
Node.js previously has a command line option
--disable-wasm-trap-handler to fully disable trap-based bound checks,
this new API would allow it to adapt automatically while keeping the
optimization in the happy path, since it's not always possible for
end users to opt-into disabling trap-based bound checks (for example,
when a VS Code Server is loaded in a remote server for debugging).
Refs: nodejs#62132
Refs: microsoft/vscode#251777
Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#105702}
Refs: v8/v8@bef0d9c
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
0c431d9 to
831617f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
831617f to
de51d96
Compare
de51d96 to
37b5c46
Compare
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The first commit is a backport of https://chromium-review.googlesource.com/c/v8/v8/+/7638233
This complements --disable-wasm-trap-handler and allows users to keep wasm functional in an environment with virtual memory restrictions (e.g. in the case of running VS code server for remote access, the user does not necessary always have the permission to configure the NODE_OPTIONS to configure --disable-wasm-trap-handler, and they don't necessarily even know this has anything to do with WASM on Node.js since they shouldn't be aware of what VS code server is implemented with).
Supersedes #60788 - this keeps the estimation logic in Node.js instead, and also keeps it in POSIX because on other systems the issue that this is solving is not practical (e.g. on Windows, one cannot bound the virtual memory available without bounding the physical memory, and it does not make a difference whether trap based bound checks are enabled or not when physical memory limit is reached)
We can tweak the threshold of estimation of needed wasm cages in 1 in Node.js to reduce the possibility of 2, this PR currently sets the threshold to 1.
Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233